Fixes To Target Traces #2016
Fixes To Target Traces #2016
Conversation
BlythMeister
commented
Jul 5, 2018
BlythMeister
commented
- Update logic for marking success/failure on Target tracing
- Allow target traces to pass string option but not be breaking change to all other traces
But maintain non breaking change for all other tag tracing
src/app/Fake.Core.Trace/Trace.fs
Outdated
| let traceTarget name description dependencyString = | ||
| traceStartTargetUnsafe name description dependencyString | ||
| traceStartTargetUnsafe name (optionDescriptionToNullable description) dependencyString |
There was a problem hiding this comment.
But this changes the public API of traceTarget. We could argue that traceTarget is for internal use only but...
There was a problem hiding this comment.
We could maybe add an traceTargetInternal and add a obsolete attribute stating "this feature is for internal use only". On the other hand others (like Xake for example) might want to use this API... So we should probably consider it public
There was a problem hiding this comment.
That was my argument 😆
I was also tempted to put the internal modifier, but then because of modules we can't since targets are not internal to trace.
There was a problem hiding this comment.
ok i'll revert this then and have the narly logic in targets :(
src/app/Fake.Core.Target/Target.fs
Outdated
| @@ -50,6 +50,10 @@ and [<NoComparison>] [<NoEquality>] TargetContext = | |||
| member x.HasError = | |||
| x.PreviousTargets | |||
| |> List.exists (fun t -> t.Error.IsSome) | |||
| member x.LastTargetError = | |||
| x.PreviousTargets | |||
| |> List.last | |||
There was a problem hiding this comment.
strictly speaking LastTargetError needs to be a bool option as the PreviousTargets list might be empty...
There was a problem hiding this comment.
oh your so right...fixing now
src/app/Fake.Core.Target/Target.fs
Outdated
| let res = runSimpleContextInternal target context | ||
| if res.HasError | ||
| if res.LastTargetError |
There was a problem hiding this comment.
Another way would be to let runSimpleContextInternal return the state of the target it executed
There was a problem hiding this comment.
ooooh even nicer!
|
Should we add a test for the scenario this fixes? |
|
It fixes the trace message...not sure how to test that... |
